Skip to content

tests(benchmark): port additional Nethermind cases #1992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Aug 12, 2025
Merged

tests(benchmark): port additional Nethermind cases #1992

merged 3 commits into from
Aug 12, 2025

Conversation

LouisTsai-Csie
Copy link
Collaborator

@LouisTsai-Csie LouisTsai-Csie commented Aug 6, 2025 •
edited
Loading

🗒️ Description

Most gas-benchmark test has been ported; more details can be found in the issue tracker. These are the new cases that need to be ported based on this branch.

  • Log0Empty
  • Log01byte
  • Log032bytes
  • Log01KiB
  • Log016KiB
  • Log4WithoutData
  • Mod1
  • ModexpMod8Exp648
  • ModexpMod8Exp896
  • ModexpMod32Exp32
  • ModexpMod32Exp36
  • ModexpMod32Exp40
  • ModexpMod32Exp64
  • ModexpMod32Exp65
  • ModexpMod32Exp128
  • ModexpMod256Exp2
  • ModexpMod264Exp2
  • ModexpMod1024Exp2
  • ModexpVulnerabilityZkevmWorst
  • EcPairing1Pair
  • EcPairing2Pairs
  • EcPairing3Pairs
  • EcPairing4Pairs
  • EcPairing5Pairs
  • EcPairing10Pairs
  • EcPairing2SetsUnique
  • Secp256r1InvalidSignature

🔗 Related Issues or PRs

Issue

✅ Checklist

  • All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks: 
    uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered adding an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
  • Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
  • Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

@LouisTsai-Csie LouisTsai-Csie self-assigned this Aug 6, 2025
@LouisTsai-Csie LouisTsai-Csie added type:test Type: Add/refactor fw unit tests; no fw or el client test case changes feature:benchmark labels Aug 6, 2025
@LouisTsai-Csie LouisTsai-Csie force-pushed the benchmark/port-extra-cases branch from 5345e70 to 55af01c Compare August 6, 2025 06:50
@LouisTsai-Csie
Copy link
Collaborator Author

More information on the LOG opcodes: we already support LOG0 to LOG4 operations, covering both zero and non-zero topics, as well as data sizes of 0 and 1 MiB. The implementation can be found here.

I think we won’t port the 1-byte, 32-byte, 1 KiB, or 16 KiB scenarios, as we try to reduce the number of tests and already include the 1 MiB cases. Some previous discussion on this topic can be found here.

@marcindsobczak
Copy link

Seems reasonable. You can ignore our Log test cases, I started working on them in Berlin, but never finished properly

@LouisTsai-Csie
Copy link
Collaborator Author

@marcindsobczak can you provide the generator for Mod1, EcPairing10Pairs and EcPairing2SetsUnique, as I can not find them in your PR.

@marcindsobczak
Copy link

Generators for it doesn't exist. Seems like I added TestCase and never delivered. Sorry for a mess

@LouisTsai-Csie
Copy link
Collaborator Author

LouisTsai-Csie commented Aug 7, 2025 •
edited
Loading

@marcindsobczak Thanks, I will request review from our team and include them in next benchmark release.

How can we collaborate to verify all the test case in gas-benchmark are fully ported?

spencer-tb
spencer-tb approved these changes Aug 11, 2025
View reviewed changes
Copy link
Contributor

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Is Secp256r1InvalidSignature missing?

fselmo
fselmo approved these changes Aug 11, 2025
View reviewed changes
Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm as well! I had the same question on Secp256r1InvalidSignature.

@LouisTsai-Csie
Copy link
Collaborator Author

LouisTsai-Csie commented Aug 12, 2025 •
edited
Loading

Thank you for your review!

Currently, we don’t have precompile tests for invalid input cases. It would be worth adding new tests for all precompiles. I’ve created an issue to track this , i might want to add it in a separate PR, as the methodology would be slightly different.

cc @spencer-tb @fselmo

@spencer-tb spencer-tb merged commit a454418 into ethereum:main Aug 12, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fselmo fselmo fselmo approved these changes

@spencer-tb spencer-tb spencer-tb approved these changes

@marioevz marioevz Awaiting requested review from marioevz

Assignees

@LouisTsai-Csie LouisTsai-Csie

Labels
feature:benchmark type:test Type: Add/refactor fw unit tests; no fw or el client test case changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LouisTsai-Csie @marcindsobczak @fselmo @spencer-tb